Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow passing asyncio_loop argument to AsyncIOLoop #3157

Merged
merged 3 commits into from
Jun 17, 2022

Conversation

minrk
Copy link
Contributor

@minrk minrk commented Jun 15, 2022

allows patterns of creating and explicitly passing the asyncio loop before instantiating the IOLoop.

For example: creating a loop with the non-default event loop policy without having to set the current policy

This is actually related to #3156 where in some of our explicit background thread event loop setup, we've been creating the asyncio event loop prior to the tornado IOLoop. To avoid relying on the deprecated asyncio.get_event_loop(), we need to pass the asyncio loop directly to the tornado IOLoop.

@@ -310,6 +310,12 @@ class AsyncIOLoop(BaseAsyncIOLoop):
Each ``AsyncIOLoop`` creates a new ``asyncio.EventLoop``; this object
can be accessed with the ``asyncio_loop`` attribute.

.. versionchanged:: 6.2
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if you want this in 6.2 or hold for 6.3. I think it's likely to be useful for folks like me trying to deal with the new deprecations, though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should definitely go into 6.2 if we do it at all.

allows patterns of creating and explicitly passing the asyncio loop before creating IOLoop

For example: creating a loop with the non-default event loop policy without having to set the current policy
@graingert
Copy link
Contributor

graingert commented Jun 15, 2022

this opens up a situation where you can have two IOLoop instances for the same asyncio loop - which causes IOLoop.current() to change value inside a running loop

import asyncio
from tornado.ioloop import IOLoop


async def main():
    loop = asyncio.get_running_loop()
    io_loop = IOLoop.current()
    other_loop = IOLoop(asyncio_loop=loop, make_current=False)
    print(f"{loop=}\n{io_loop=}\n{other_loop=}\n{IOLoop.current()=}")


asyncio.run(main())
loop=<_UnixSelectorEventLoop running=True closed=False debug=False>
io_loop=<tornado.platform.asyncio.AsyncIOMainLoop object at 0x7fac54b4b940>
other_loop=<tornado.platform.asyncio.AsyncIOLoop object at 0x7fac549efc40>
IOLoop.current()=<tornado.platform.asyncio.AsyncIOLoop object at 0x7fac549efc40>

I think this should raise if there's already a IOLoop in the IOLoop._ioloop_for_asyncio dictionary for the loop passed into asyncio_loop, but I'm not sure if this can be made threadsafe

@minrk
Copy link
Contributor Author

minrk commented Jun 16, 2022

That condition already exists if you instantiate AsyncIOMainLoop multiple times:

import asyncio
from tornado.platform.asyncio import AsyncIOMainLoop

async def main():
    loop1 = AsyncIOMainLoop()
    loop2 = AsyncIOMainLoop()
    assert loop1.asyncio_loop is asyncio.get_running_loop()
    assert loop1.asyncio_loop is loop2.asyncio_loop
    assert loop1 is not loop2


if __name__ == "__main__":
    asyncio.run()

I'm not sure such the check needs to be perfect, or indeed threadsafe, since it would require user code to explicitly pass a single asyncio loop to two IOLoop constructors in different threads. The most likely case is multiple paths to initialization, which accidentally get taken twice, which should be handled by the check I've added here.

black seems to have changed its mind
Copy link
Member

@bdarnell bdarnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way I solved this in tornado/testing.py was to use asyncio_loop.run_until_complete to activate the asyncio loop and construct a tornado loop with IOLoop.current().

This does seem a little nicer - the run_until_complete solution is cumbersome and non-obvious. But on the other hand run_until_complete avoids questions about creating multiple tornado loops for the same asyncio loop and who closes which loop.

What do you think about the run_until_complete alternative? Do you still think it's worth creating a new variation of the constructor?

@@ -310,6 +310,12 @@ class AsyncIOLoop(BaseAsyncIOLoop):
Each ``AsyncIOLoop`` creates a new ``asyncio.EventLoop``; this object
can be accessed with the ``asyncio_loop`` attribute.

.. versionchanged:: 6.2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should definitely go into 6.2 if we do it at all.

@graingert
Copy link
Contributor

graingert commented Jun 17, 2022

The way I solved this in tornado/testing.py was to use asyncio_loop.run_until_complete to activate the asyncio loop and construct a tornado loop with IOLoop.current().

This does seem a little nicer - the run_until_complete solution is cumbersome and non-obvious. But on the other hand run_until_complete avoids questions about creating multiple tornado loops for the same asyncio loop and who closes which loop.

What do you think about the run_until_complete alternative? Do you still think it's worth creating a new variation of the constructor?

This would work if there was a public way to reset the threading ident

def reset_threading_ident():
    asyncio.get_running_loop()
    IOLoop.curent()._threading_identity = threading.get_ident()

async def call(fn):
    return fn()
class IOLoopInThread(Thread):
    def __init__(self):
        runner = asyncio.Runner()
        self._event = asyncio.Event() # LoopBoundMixin in 3.10
        self._io_loop = runner.run(call(IOLoop.current))

    def run(self):
        with self._runner as runner:
            runner.run(call(reset_threading_ident))
            runner.run(self._event.wait())

    def stop(self):
        self._runner.get_loop().call_soon_threadsafe(self._event.set)
        self.join()

@bdarnell
Copy link
Member

Ah, right, the thread imprinting is a good reason to avoid run_until_complete, so this PR looks like the right solution to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants